Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up non-functional CloudEvents Metrics in Reconciler for Deprecated CloudEvents #6827

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Clean up non-functional CloudEvents Metrics in Reconciler for Deprecated CloudEvents #6827

merged 1 commit into from
Jun 14, 2023

Conversation

JeromeJu
Copy link
Member

Changes

This commit cleans up the CloudEvents Metrics which are no longer functional with the removal of CloudEvents PipelineResources, and also the CloudEvents field of the TaskRun Status API is also deprecated and not moved to v1.

/kind cleanup
fixes: #6826

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 14, 2023
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 14, 2023
@JeromeJu
Clean up non-functional CloudEvents Metrics for Deprecated CloudEvents
cdc9134 
This commit cleans up the CloudEvents Metrics which are no longer
functional with the removal of CloudEvents PipelineResources, and also
the CloudEvents field of the TaskRun Status API is also deprecated and not
moved to v1.
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 84.7% 84.9% 0.2
pkg/taskrunmetrics/metrics.go 84.9% 83.0% -1.9

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 84.7% 84.9% 0.2
pkg/taskrunmetrics/metrics.go 84.9% 83.0% -1.9

@JeromeJu JeromeJu changed the title Clean up non-functional CloudEvents Metrics for Deprecated CloudEvents Clean up non-functional CloudEvents Metrics in Reconciler for Deprecated CloudEvents Jun 14, 2023
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 84.7% 84.9% 0.2
pkg/taskrunmetrics/metrics.go 84.9% 83.0% -1.9

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 84.7% 84.9% 0.2
pkg/taskrunmetrics/metrics.go 84.9% 83.0% -1.9

@JeromeJu JeromeJu mentioned this pull request Jun 14, 2023
Merged
7 tasks
@JeromeJu
Copy link
Member Author

/assign @afrittoli @dibyom @lbernick

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2023
@lbernick
Copy link
Member

/lgtm

I think we could also remove some of the unused cloudevents code:

pipeline/pkg/reconciler/events/cloudevent/cloud_event_controller.go

Lines 38 to 52 in f0f5646

func EmitCloudEvents(ctx context.Context, object runtime.Object) {
logger := logging.FromContext(ctx)
configs := config.FromContextOrDefaults(ctx)
sendCloudEvents := (configs.Defaults.DefaultCloudEventsSink != "")
if sendCloudEvents {
ctx = cloudevents.ContextWithTarget(ctx, configs.Defaults.DefaultCloudEventsSink)
}
if sendCloudEvents {
err := SendCloudEventWithRetries(ctx, object)
if err != nil {
logger.Warnf("Failed to emit cloud events %v", err.Error())
}
}
}

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@JeromeJu
Copy link
Member Author

/lgtm

I think we could also remove some of the unused cloudevents code:

pipeline/pkg/reconciler/events/cloudevent/cloud_event_controller.go

Lines 38 to 52 in f0f5646

func EmitCloudEvents(ctx context.Context, object runtime.Object) {
logger := logging.FromContext(ctx)
configs := config.FromContextOrDefaults(ctx)
sendCloudEvents := (configs.Defaults.DefaultCloudEventsSink != "")
if sendCloudEvents {
ctx = cloudevents.ContextWithTarget(ctx, configs.Defaults.DefaultCloudEventsSink)
}
if sendCloudEvents {
err := SendCloudEventWithRetries(ctx, object)
if err != nil {
logger.Warnf("Failed to emit cloud events %v", err.Error())
}
}
}

Agreed! as put at #6826 (comment), this current PR is opened to unblock v1 swap and as pointed these could also be removed in a followup PR.

@JeromeJu
Copy link
Member Author

/retest
test: TestExamples/v1/taskruns/steptemplate-env-merge with
Could not get logs for pod steptemplate-env-merge-756np-pod: container "step-foo" in pod "steptemplate-env-merge-756np-pod" is waiting to start: image can't be pulled

@dibyom
Copy link
Member

dibyom commented Jun 14, 2023 

 message: 'The step "unnamed-0" in TaskRun "retry-example-lqdlp-hello-world" failed

          to pull the image "". The pod errored with the message: "Back-off pulling image
          "busybox"."'

@dibyom
Copy link
Member

dibyom commented Jun 14, 2023

/test pull-tekton-pipeline-alpha-integration-tests

@JeromeJu
Copy link
Member Author

JeromeJu commented Jun 14, 2023 •
edited
Loading

/test pull-tekton-pipeline-alpha-integration-tests
potential flake: TestExamples/v1/pipelineruns/using-retries-and-retry-count-variables, reached retries count limit pulling which should not be related to the current chagnes.

@tekton-robot tekton-robot merged commit 9605dd1 into tektoncd:main Jun 14, 2023
khrm added a commit to khrm/pipeline that referenced this pull request Jun 19, 2023
@khrm
Remove cloudevent metrics from documentation
c1c5842 
CloudEvent metrics was removed as part of pr tektoncd#6827 . We removed this
from documentation also.
@khrm khrm mentioned this pull request Jun 19, 2023
Merged
7 tasks
tekton-robot pushed a commit that referenced this pull request Jun 20, 2023
@khrm @tekton-robot
Remove cloudevent metrics from documentation
5e93dd3 
CloudEvent metrics was removed as part of pr #6827 . We removed this
from documentation also.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibyom dibyom dibyom approved these changes

@pritidesai pritidesai Awaiting requested review from pritidesai

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@afrittoli afrittoli

@dibyom dibyom

@lbernick lbernick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup Metrics for the Deprecated CloudEvents
5 participants
@JeromeJu @tekton-robot @lbernick @dibyom @afrittoli